Skip to content

Add config-matrix tests in enforce_distribution.rs for range-satisfaction settings#23627

Open
blinding-pixels wants to merge 1 commit into
apache:mainfrom
blinding-pixels:agent/range-satisfaction-config-matrix
Open

Add config-matrix tests in enforce_distribution.rs for range-satisfaction settings#23627
blinding-pixels wants to merge 1 commit into
apache:mainfrom
blinding-pixels:agent/range-satisfaction-config-matrix

Conversation

@blinding-pixels

Copy link
Copy Markdown

Which issue does this PR close?

Rationale for this change

The existing SQL logic tests exercise Range reuse through aggregates, joins, and windows. Repeating the same configuration permutations for every operator would make those tests large and difficult to maintain.

Issue #23572 describes six wildcard rules across four dimensions:

  • Exact, subset, or incompatible keys
  • Subset threshold met or not met
  • Preserve-file threshold met or not met
  • Target partition count equal to or greater than the input count

Those rules directly specify 15 of the 24 possible combinations. The remaining nine expectations are derived from the documented semantics rather than recorded from optimizer output:

  • Exact Range keys satisfy the requirement directly in range_satisfies_key_partitioning.
  • Subset keys require allow_subset_satisfy_partitioning.
  • Subset satisfaction is enabled when the subset threshold is met, or when the preserve threshold is met and increasing the target partition count is the only reason to repartition.
  • Incompatible keys never satisfy the requirement.

This explains why these cases have different outcomes:

  • Exact key, threshold not met, target equal: reuse
  • Subset key, threshold not met, preserve not met, target equal: hash

The equal partition count is not a global reuse shortcut. The input must first satisfy the distribution requirement. Exact keys satisfy directly, while subset keys require permission.

What changes are included in this PR?

The matrix uses one RangeSatisfactionConfigCase structure and eight configuration rows:

  • Two subset-threshold states
  • Two preserve-threshold states
  • Two target-partition states

Each row contains explicit expected results for exact, subset, and incompatible keys, producing all 24 combinations.

For every combination, the test:

  1. Creates the same four-partition Range input.
  2. Applies the selected key relationship and optimizer configuration.
  3. Runs the real physical optimizer.
  4. Checks whether the optimized plan contains a hash repartition.

The expected values remain explicit in the table rather than being calculated using a second implementation of the optimizer logic.

I also added KeyPartitioningRequirementExec under the existing physical-plan test utilities. It is a neutral single-input operator that requests keyed partitioning and enables the existing Range opt-in. This lets the matrix test the shared decision without aggregate, join, or window behavior affecting the result.

I considered several test harnesses before using the neutral operator:

  • An aggregate harness introduced aggregate-specific round-robin repartitioning.
  • A window harness worked but coupled the shared matrix to behavior already tested by window-specific cases.
  • OutputRequirementExec and the existing MockReqExec request ordinary keyed distributions but cannot enable the crate-private Range opt-in.

Are these changes tested?

Yes.

The following checks pass:

  • The complete 24-combination configuration matrix
  • All related Range physical-optimizer tests
  • The complete range_partitioning.slt file
  • Formatting and diff checks
  • Targeted Clippy checks

Matrix mutation testing

The nine expectations not directly specified by the issue were mutation-tested against their decision branches:

Mutation location and edit Result
datafusion/physical-plan/src/distribution_requirements.rs:459: changed PartitioningSatisfaction::Exact to PartitioningSatisfaction::NotSatisfied All three derived exact-key cells failed, along with the issue-specified exact cases using the same rule
datafusion/physical-optimizer/src/ensure_requirements/enforce_distribution.rs:1305: appended && false to allow_subset_satisfy_partitioning All five derived subset cells expecting reuse failed: the four threshold-met combinations and the preserve-met/greater-target combination
datafusion/physical-optimizer/src/ensure_requirements/enforce_distribution.rs:1303: changed current_partitions < target_partitions to current_partitions <= target_partitions Exactly one cell failed: subset key, threshold not met, preserve met, target equal

The final mutation changed one character and flipped only one matrix cell. This shows that the matrix distinguishes the preserve/target boundary rather than several cells merely depending on the same broad condition.

All mutations were restored, and the complete matrix passed again.

Operator opt-in mutation testing

I also independently removed each production Range opt-in and ran range_partitioning.slt:

Mutated operator and location SLT failures
AggregateExecdatafusion/physical-plan/src/aggregates/mod.rs:1952 5 — range_partitioning.slt:41, :101, :137, :218, and :551
HashJoinExecdatafusion/physical-plan/src/joins/hash_join/exec.rs:1297 5 — range_partitioning.slt:280, :476, :514, :551, and :607
BoundedWindowAggExecdatafusion/physical-plan/src/windows/bounded_window_agg_exec.rs:336 2 — range_partitioning.slt:701 and :821
WindowAggExecdatafusion/physical-plan/src/windows/window_agg_exec.rs:244 1 — range_partitioning.slt:730

Each mutation introduced unexpected hash repartitions. Line 551 protects both AggregateExec and HashJoinExec.

This confirms that the two test layers protect different failure points:

  • The neutral matrix protects the shared configuration decision.
  • The SLT cases protect each operator’s opt-in.

When the SLT permutations are slimmed, the load-bearing operator cases above should remain. I would appreciate guidance on whether they should be marked as contract tests in place or moved into a dedicated section or file.

Are there any user-facing changes?

No query-planning or runtime behavior changes are included.

The only new exposed type is a test utility under the existing datafusion_physical_plan::test module. I did not introduce a public Range-policy API because the current opt-in is documented as a temporary bridge pending #23266.

@github-actions github-actions Bot added core Core DataFusion crate physical-plan Changes to the physical-plan crate labels Jul 16, 2026
@blinding-pixels
blinding-pixels marked this pull request as ready for review July 17, 2026 01:12
@blinding-pixels

Copy link
Copy Markdown
Author

Hi @gabotechs, this is my first contribution to DataFusion. Would you be able to trigger CI and review this PR when you have a chance? I’m happy to explain any of the decisions I made here. I’m looking forward to being a useful member of the community. Thank you!

@gene-bordegaray gene-bordegaray left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

overall looks good, thanks for banging this one out, this is very very much appreaciated 🙇

One goal was to keep range_partitioning.slt a bit leaner. I think we can do some of that now by eliminating some repetitive tests. I was thnking:

  • TEST 4: Exact Range Aggregate Below Subset Threshold
  • TEST 5: Range Subset Aggregate Rehashes Below Subset Threshold
  • TEST 6: Aggregate Rehashes Below Subset Threshold
  • TEST 7: Aggregate Preserves Range When Preserve File Threshold Met
  • TEST 8: Aggregate Rehashes When Preserve File Threshold Not Met
  • TEST 13: Compatible Range Join Repartitions to Increase Parallelism
  • TEST 14: Preserve File Partitions Preserves Range Join Inputs

cc: @gabotechs let me know what you think. This reduces the noise in that file to focus on range end to end and this unit now covers all the tricky "above this" "below that" distribution stuff 👍

let plan = config.to_plan(requirement, &DISTRIB_DISTRIB_SORT);
let plan = displayable(plan.as_ref()).indent(true).to_string();
let has_hash_repartition =
plan.contains("RepartitionExec: partitioning=Hash");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should be checking for any repartition. For example we can have a RoundRobin inserted if we choose to increase parallelism when less than target_partitions and threshld values

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core Core DataFusion crate physical-plan Changes to the physical-plan crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add config-matrix tests in enforce_distribution.rs for range-satisfaction settings

2 participants